Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Future - Create a CRA framework #18695

Closed
wants to merge 10 commits into from
Closed

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Jul 12, 2022

Replace the preset-create-react-app with framework/cra
This framework is what we expect users to use if they use CRA for their app.
This framework has portions of preset-create-react-app copied into it, and is self-containing, it does not reference preset-create-react-app at all. It's also been significantly reduced in complexity, though this means we have to battle-test it. It's likely not compatible with all version of CRA.

I updated all the CRA examples to use this new framework.

I updated the CLI a bit, though this might need more work, I did not test this yet.

Documentation needs more work.

@yannbf raised the question of what this framework should be called.. "@storybook/cra" vs "@storybook/create-react-app"?

@nx-cloud
Copy link

nx-cloud bot commented Jul 12, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d764cdf. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@ndelangen ndelangen self-assigned this Jul 12, 2022
@ndelangen ndelangen added cra Prioritize create-react-app compatibility maintenance User-facing maintenance tasks multiframework feature request and removed maintenance User-facing maintenance tasks labels Jul 12, 2022
addons/docs/src/preset.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,89 @@
{
"name": "@storybook/cra",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to call it cra or create-react-app like it was done for the preset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinion on this, but a slight preference fro "cra"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kylegach thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer to Norbert.


## Create React App

TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO!

frameworks/cra/package.json Outdated Show resolved Hide resolved
frameworks/cra/package.json Outdated Show resolved Hide resolved
@ndelangen ndelangen changed the title create a cra framework Future - Create a CRA framework Jul 12, 2022
@ndelangen ndelangen force-pushed the future/improve-preview-perf branch from 77a46f6 to 9d4a720 Compare July 15, 2022 07:38
@ndelangen ndelangen changed the base branch from future/improve-preview-perf to future/base July 18, 2022 18:00
package.json Outdated Show resolved Hide resolved
@ndelangen
Copy link
Member Author

Looks like this is quite close. Seems like some CSS loader misconfiguration, and some color control acting weird

Base automatically changed from future/base to next July 25, 2022 10:37
# Conflicts:
#	examples/cra-kitchen-sink/package.json
#	examples/cra-ts-essentials/package.json
#	examples/cra-ts-kitchen-sink/package.json
#	examples/html-kitchen-sink/package.json
#	lib/cli/src/versions.ts
#	yarn.lock
@yannbf
Copy link
Member

yannbf commented Aug 3, 2022

Hey @ndelangen shall we merge next into this and check what's the next step?

@ndelangen
Copy link
Member Author

not for a while @yannbf, I'll get back to this later.

I did create these:
#18848
#18851

In an attempt to pave the way for this to land some day.

I think it's close, but I need to look real careful at the change I made to how the paths are resolved.

options: {
...mdxLoaderOptions,
skipCsf: false,
Object.defineProperty(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the idea behind this construct?

Copy link
Member Author

@ndelangen ndelangen Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add a hidden property to the rule object, so it's easier to identify when you want to filter them out

const { presets, configDir } = options;
const frameworkOptions = await presets.apply<FrameworkOptions>('frameworkOptions');

const CRAPath = await getCRAPath(frameworkOptions.scriptsPackageName, options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

capitalization?

),
path.dirname(require.resolve(path.join('@storybook/web-components', 'package.json'))),
dirname(require.resolve(join('@storybook/preset-web-components-webpack', 'package.json'))),
dirname(require.resolve(join('@storybook/web-components', 'package.json'))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use join and not just /?


```sh
cd my-react-app
npx sb init
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
npx sb init
npx storybook init

kylegach added a commit that referenced this pull request Aug 17, 2022
kylegach added a commit that referenced this pull request Aug 17, 2022
],
"scripts": {
"check": "tsc --noEmit",
"prepare": "esrun ../../scripts/prepare/bundle.ts"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the other frameworks, I think this should be:

Suggested change
"prepare": "esrun ../../scripts/prepare/bundle.ts"
"prepare": "esrun ../../../scripts/prepare/bundle.ts"

@@ -51,6 +51,7 @@ const getFrameworkDetails = (
builder?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndelangen — FYI, I ended up making a separate PR with this (slightly modified) change: #18968

@ndelangen ndelangen marked this pull request as draft October 5, 2022 21:22
@ndelangen ndelangen closed this Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cra Prioritize create-react-app compatibility feature request multiframework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants